Skip to content

Conversation

@rokleM
Copy link
Contributor

@rokleM rokleM commented Dec 7, 2016

Adds support for ARGB packed pixels.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great PR! happy to merge it. Thanks!

@@ -1,0 +1,3 @@
[*.cs]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea.

Assert.Equal(bgra, new byte[] { 0, 0, 0, 128 });
}

[Fact]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

/// <returns>The <see cref="uint"/></returns>
static uint Pack(float x, float y, float z, float w)
{
var value = new Vector4(x, y, z, w);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use var myself but I can clean this up after the merging the PR so don't worry about it.

Copy link
Contributor Author

@rokleM rokleM Dec 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, didn't see enough of the rest of the code to not use it (I do use var)!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not using var? 😄 The rule I use these days is: use var by default, unless the type cannot be easily inferred from reading the code. It makes the code more maintainable - for instance if I change from an IEnumerable to IList, etc, I don't need to update all the places, it kinda just works.

Copy link
Member

@JimBobSquarePants JimBobSquarePants Dec 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olivif What is the resultant type of x in this bit of code?

var x = (byte)1 + (byte)2;

I'm sure you know this already but it's an int due to implicit casting since bytes don't have an + operator. (This is the simplest example I could think of as an easy mistake to make using var)

I work with streams a lot and I've lost count of the amount of times that the wrong value has been passed to a BinaryWriter due to the use of var. That's always a nightmare to debug!

I know it's more common to use now but in certain critical codebases like the Roslyn compiler it's still banned.

I honestly believe that it's having a negative effect on modern programming. Devs aren't learning their types nor the framework properly as a result and are being careless, using var mostly because they don't know what the return type of a method should be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JimBobSquarePants this is super interesting, I've actually never run into these type problems due to var, thanks for sharing 😄 . I understand the perils now, up until now it was just the maintainability argument for me, but now I see how it could go wrong.

I honestly believe that it's having a negative effect on modern programming. Devs aren't learning their types nor the framework properly as a result and are being careless, using var mostly because they don't know what the return type of a method should be.

I fully agree here. I was fortunate enough to write a lot of C++ before coming to C# but I realize that might not be the norm these days.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're better than I then! I tried C++ once..... Nope, too hard! 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha! Well, it was great at the time and definitely learnt a lot. Would I go back to unmanaged without a really good reason? Probably not 😄

@JimBobSquarePants JimBobSquarePants merged commit 90d0b37 into SixLabors:master Dec 8, 2016
@JimBobSquarePants
Copy link
Member

Excellent! Thanks for this 💯

@rokleM
Copy link
Contributor Author

rokleM commented Dec 8, 2016

Thank you for making the library happen! 😄

@rokleM rokleM deleted the argb branch December 8, 2016 13:56
antonfirsov pushed a commit that referenced this pull request Jan 18, 2020
Apply SharedInfrastructure, introduce shared guards
JimBobSquarePants added a commit that referenced this pull request Feb 17, 2021
Apply SharedInfrastructure, introduce shared guards
JimBobSquarePants added a commit that referenced this pull request Feb 17, 2021
Apply SharedInfrastructure, introduce shared guards
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants